-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Add CI check to ensure that rustdoc JSON FORMAT_VERSION
is correctly updated
#142677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add CI check to ensure that rustdoc JSON FORMAT_VERSION
is correctly updated
#142677
Conversation
|
Could this be a tidy check instead? It's better to have these things in one centralized place in Rust, rather than scattered around in random bash scripts. |
I guess? Didn't even realize that I could do the same in rust much more easily. ^^' |
This comment has been minimized.
This comment has been minimized.
23e5243
to
5d9e058
Compare
Moved it to |
Oh also: r? @Kobzol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it could also work locally, but it's always a PITA to figure out a diff vs master, so this is fine. Could you please try to break the CI job in this PR to see if it works on PR CI?
5d9e058
to
43d18cf
Compare
rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing |
Looks like the check didn't run, hmm. Probably adding some logging before the |
It's because |
76cea70
to
122cf96
Compare
This comment has been minimized.
This comment has been minimized.
122cf96
to
c352ab5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conceptually this change makes sense and is good.
This is a first version. I plan to send a follow-up to also ensure that
FORMAT_VERSION
is updated if any code change is done inrustdoc-json-types
. For that I just need to check that a line not starting with/
and not an empty line was updated.
I worry that this could still have false positives. E.g. #142642 touches non-comments in src/rustdoc-json-types
, but doesn't need to bump the FORMAT_VERSION
src/rustdoc-json-types/lib.rs
Outdated
@@ -38,7 +38,7 @@ pub type FxHashMap<K, V> = HashMap<K, V>; // re-export for use in src/librustdoc | |||
// are deliberately not in a doc comment, because they need not be in public docs.) | |||
// | |||
// Latest feature: Pretty printing of inline attributes changed | |||
pub const FORMAT_VERSION: u32 = 48; | |||
pub const FORMAT_VERSION: u32 = 49; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This 48->49 change should be reverted before merging.
c352ab5
to
5c4daf1
Compare
5c4daf1
to
9cccf1e
Compare
fee5757
to
115523e
Compare
This comment has been minimized.
This comment has been minimized.
5efb3f8
to
ae4dbd4
Compare
This comment has been minimized.
This comment has been minimized.
ae4dbd4
to
cb6460f
Compare
This comment has been minimized.
This comment has been minimized.
cb6460f
to
aa1b8eb
Compare
This comment has been minimized.
This comment has been minimized.
aa1b8eb
to
6367694
Compare
Check is failing as expected so removed the failing commit. Should be now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two nits, otherwise looks good.
src/tools/tidy/src/rustdoc_json.rs
Outdated
) { | ||
Ok(Some(commit)) => commit, | ||
Ok(None) => { | ||
*bad = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please only set *bad
here and below on CI (if CiEnv::current()
is CI)? For some users get_closest_upstream_commit
might not return anything, e.g. if they have a shallow checkout. Yes, this is all sadly very annoying.. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep discovering new cases to handle 😆
src/tools/tidy/src/rustdoc_json.rs
Outdated
*bad = true; | ||
if latest_feature_comment_updated { | ||
eprintln!( | ||
"error: `Latest feature` comment was updated whereas `FORMAT_VERSION` wasn't" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although people will probably be able to infer it, I think it would be nice if the error message contained the filename that should be fixed, otherwise the error can be quite opaque.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Applied your suggestions. Thanks a lot, learned a lot with this PR! |
This comment has been minimized.
This comment has been minimized.
Only emit git errors if we are in CI environment
989fd9d
to
0fc9507
Compare
Yeah, there's really a lot to learn here 😆 Thanks! @bors r+ rollup |
Follow-up of #142601.
Tested it locally with:
BASE_COMMIT=1bb335244c311a07cee165c28c553c869e6f64a9 src/ci/docker/host-x86_64/mingw-check-1/validate-rustdoc-json-format-version-update.sh
(whereBASE_COMMIT
value was the commit before I made a wrong change with theFORMAT_VERSION
update).This is a first version. I plan to send a follow-up to also ensure that
FORMAT_VERSION
is updated if any code change is done inrustdoc-json-types
. For that I just need to check that a line not starting with/
and not an empty line was updated. Fun times withgrep
ahead. :')cc @aDotInTheVoid
r? @nnethercote